Skip to content

fix(claude): keep a marker for tool_result document blocks - #939

Closed
DevMello wants to merge 1 commit into
lidge-jun:devfrom
DevMello:fix/tool-result-document-blocks
Closed

fix(claude): keep a marker for tool_result document blocks#939
DevMello wants to merge 1 commit into
lidge-jun:devfrom
DevMello:fix/tool-result-document-blocks

Conversation

@DevMello

@DevMello DevMello commented Aug 3, 2026

Copy link
Copy Markdown

Summary

When a tool result contains a document block, the inbound translation dropped it and routed providers received an empty tool output, so the model concluded the tool returned nothing. A document in a plain user message already gets a [document: title] marker; the tool-result path now emits the same marker.

Verification

  • New test in tests/claude-inbound.test.ts: titled, untitled, and mixed text plus document results.
  • Reproduced against a mock upstream: the tool message arrived as "" before the fix and [document] after.
  • bun run test, typecheck, lint:gui, privacy:scan.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

  • Bug Fixes

    • Document attachments in tool results are now represented correctly instead of producing empty output.
    • Document titles are preserved when available; untitled documents use a clear placeholder.
  • Tests

    • Added coverage to verify document handling, text preservation, and parseable tool results.

A tool_result whose content is a document block translated to an
empty tool output, so routed models treated the attachment as a tool
that returned nothing. Surface the same "[document: title]" marker
the user-message path already emits.
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

Tool-result document handling

Layer / File(s) Summary
Document marker conversion and validation
src/claude/inbound.ts, tests/claude-inbound.test.ts
Tool-result document blocks now become [document: <title>] or [document] text markers. The regression test verifies text preservation, Responses input text conversion, and successful request parsing.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: ingwannu, lidge-jun, wibias

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: preserving markers for document blocks in Claude tool results.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@lidge-jun

Copy link
Copy Markdown
Owner

Carried onto the review stack as #953 (stack 3/3), unmodified.

Your commits were taken with git cherry-pick -x, so they keep your authorship — git log --format='%an' on the stack branch shows you, not me. No content was changed; the diff on the stack is byte-identical to what you wrote here, and it applied to dev with no conflict resolution.

Verified on the stack: bun x tsc --noEmit exit 0, and the full suite at 7691 pass / 8 skip / 0 fail across 507 files.

This PR stays open until #953 lands. If a maintainer prefers to take yours directly instead, that path is unaffected — the stack commits get dropped and this one merges. Once #953 merges I'll close this as carried, with the credit already in the commit history rather than in a comment.

Stack: #951 (plan, base dev) → #952 (#908 long-context pricing) → #953 (this carry). Review bottom-up.

Thanks for the fix.

@Wibias

Wibias commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Leave it as draft. Thanks. It doesnt need any more changes as you can see in juns comment on which you didn't respond to.

@lidge-jun

Copy link
Copy Markdown
Owner

Closing this as carried into #953, not as rejected. Your fix is shipping — this is bookkeeping so the same change isn't sitting in the review queue twice.

Your commit(s) are on the stack branch as c233a7cf6, cherry-picked with git cherry-pick -x. Authorship is preserved, so git log --format='%an' shows you rather than me, and I verified the carried diff is byte-identical to yours rather than assuming it:

$ git show <yours> | git patch-id --stable
$ git show <carried> | git patch-id --stable
IDENTICAL

No content was changed and no conflict resolution was needed.

One thing I want to be straight about: my earlier comment said I'd close this after #953 merged, and I'm closing it before. #953 is open and reviewable now, so closing early is my call as maintainer to keep the queue honest — not a signal that the review finished. If the stack doesn't land, reopening this is one click, and the branch is untouched.

Verified on the stack: bun x tsc --noEmit exit 0, full suite 7729 pass / 8 skip / 0 fail across 508 files.

Stack: #951 (plan, base dev) → #952#953 (this carry) → #954#955. Review bottom-up.

Thanks @DevMello — this was a clean fix and it's in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants